Skip to content

Support DAE models (DFN, lead_acid.Full) in CellElectrical and CellElectrothermal - #22

Open
DavidMStraub wants to merge 4 commits into
pathsim:masterfrom
DavidMStraub:dae-cells
Open

DavidMStraub wants to merge 4 commits into
pathsim:masterfrom
DavidMStraub:dae-cells

Conversation

@DavidMStraub

@DavidMStraub DavidMStraub commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #18. With the reduced Jacobian, constraint tolerance and output function now in PathSim (0.27.1), the monolithic cell blocks are based on SemiExplicitDAE, so models with algebraic variables after discretisation (DFN, lead_acid.Full, LOQS on PyBaMM 26.7) no longer raise NotImplementedError and are integrated by PathSim's own solvers. ODE models such as SPMe take the same path without algebraic states; results are unchanged.

Changes

  • _CellBase derives from SemiExplicitDAE, passes the analytic CasADi Jacobians and computes the cell outputs and voltage cut-offs in func_out; only public PathSim API is used.
  • New tolerance parameter (default 1e-6) for the algebraic solve.
  • Requires pathsim>=0.27.1.
  • Tests: DAE-raises tests replaced by DFN and lead_acid.Full tests, including voltage comparisons against PyBaMM's own solver.
  • README: model compatibility table replaced by a short list, since all blocks now support all models. Notebook 03 updated accordingly.

Notes

  • 600 s discharge at 5 A, final voltage vs PyBaMM: SPMe 3.8198/3.8198 V (0.6 s), DFN 3.8237/3.8237 V (73 s), lead_acid.Full 2.1301/2.1304 V (1.9 s).
  • The tolerance default is a compromise: lead_acid.Full stalls at 1e-7 and below due to ill-conditioned algebraic equations, while DFN runs about twice as fast at 1e-9.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Empty algebraic states currently break pure ODE models, and the DFN notebook contradicts the updated compatibility documentation.

Review effort: Balanced
Findings: 1 High severity · 1 Low severity

Open (2)
What changed in this PR

Adds monolithic DAE support for PyBaMM cell models using PathSim’s SemiExplicitDAE.

Changes:

  • Integrates algebraic states with analytic CasADi Jacobians and configurable tolerance.
  • Adds DFN and lead-acid DAE coverage.
  • Updates compatibility documentation and requires PathSim 0.26.
File Description
src/​pathsim_batt/​cells/​pybamm_cell.py Implements monolithic DAE integration.
tests/​cells/​test_pybamm_cell.py Adds DFN integration tests.
tests/​cells/​test_lead_acid.py Enables monolithic lead-acid DAE tests.
README.md Updates model compatibility guidance.
pyproject.toml Raises the PathSim minimum version.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/pathsim_batt/cells/pybamm_cell.py Outdated
Comment thread README.md Outdated
@DavidMStraub
DavidMStraub marked this pull request as ready for review October 12, 2026 07:29
@DavidMStraub
DavidMStraub requested a balanced review from Copilot October 12, 2026 07:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Public documentation incorrectly claims compatibility with every PyBaMM battery model despite mandatory output-variable requirements.

0 open findings

2 resolved since last review
Previously missed (2)

In code that hasn't changed since last review

Low severity Qualify compatibility claim for models missing required outputs

README.md:69

This compatibility claim is broader than the block API. Construction still resolves mandatory voltage/heating (and, for electrothermal, temperature) outputs via _resolve_output_vars; for example, PyBaMM's sodium_ion.BasicDFN has no Total heating [W] output, so CellElectrical raises instead of accepting it. Please qualify support to models exposing the selected block's required outputs.

Low severity Qualify documentation claim of universal PyBaMM model support

src/​pathsim_batt/​cells/​pybamm_cell.py:719

The wrapper does not handle literally any PyBaMM model even if PyBaMM's solver can integrate it: _resolve_output_vars still requires voltage, heating, and SOC outputs. Qualify this statement so the public class documentation does not promise compatibility that construction rejects.

This issue also appears on line 755 of the same file.

🧠 Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for DAE models with PathSim solvers

2 participants